Add Edit Network Interface Modal#985
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Edit: The idea of disabling it because its already primary feels unusual to me, and editing other NICs within another NIC's pane also. Since to make one primary, you would need to demote another. Perhaps better suited for a modal action outside of the pane, as in a button that is "Set Primary NIC", with radio buttons or a dropdown. |
|
and it goes a lil' something like this (ignore that it's selecting the current selected item) with edit pane - not sure what we call the 'Primary' field. How is this reflected in the API? A boolean? |
|
A radio button certainly communicates the exclusivity. And yes, there’s exactly one primary interface, and all others are secondary. I like that UI, keeping the setting of the primary distinct from the other updates.
… On 28 Jun 2022, at 03:19, Benjamin Leonard ***@***.***> wrote:
and it goes a lil' something like this (ignore that it's selecting the current selected item)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
|
|
@benjaminleonard, primary is a Boolean, yes. |
Cool – I ask because the column name is Primary, I like to have text not just the checkmark icon so we're not relying entirely on the icon for meaning. Not that it's particularly ambiguous. So currently it says Primary: Primary. Idk if there's a better way of doing that, it's probably fine for now. |
|
Oh, also, we can't change the ip address either. It's only the name and description that are editable. |
|
I've added the action to make a NIC primary in the row actions menu. I'll create an issue to do a follow up. The are questions around the modal approach like what columns besides name would be needed in the modal to make the decision to switch which is primary. We also need to figure out how to manage what's the primary interface on instance create when making a custom NIC.
|
The primary NIC is always the first one, if multiple are specified at instance-creation time. Really, the first interface to be attached to an instance is the primary, no matter when they're specified (instance create, or later with new requests to add a NIC to an instance). |
|
I updated the UI of the table itself to be closer to the storage tab (complete with text that says editing can only happen when the instance is stopped)
|
| // Close toast, it holds up the test for some reason | ||
| await page.click('role=button[name="Dismiss notification"]') | ||
| await expectNotVisible(page, ['role=cell[name="nic-2"]']) | ||
| await expectNotVisible(page, ['role=cell[name="nic-3"]']) |
| </div> | ||
| <h2 id="network-interfaces" className="mb-4 text-mono-sm text-secondary"> | ||
| Network Interfaces | ||
| </h2> |
There was a problem hiding this comment.
This matches what's on storage, and I agree we need something to say what's in the table, but I think it's too small (which probably means the disks ones are too small too?) @benjaminleonard
There was a problem hiding this comment.
I'll leave that for a polish follow up
| const text = cellTexts[i] | ||
| if (text === null) { | ||
| continue | ||
| } |
| invariant( | ||
| instanceName, | ||
| 'instanceName is required when posting a network interface' | ||
| ) |
There was a problem hiding this comment.
Do you anticipate using this form in a context where we don't have an instanceName? Otherwise this is basically the same as including 'instanceName' in the useParams() on line 21.
I also don't see why we need an invariant on interfaceName since it's a required field in the form — the invariant is enforced by form validation.
There was a problem hiding this comment.
If it's used in the instance-create-form (via the custom NIC option) then instanceName wouldn't be in the path.
There was a problem hiding this comment.
As for the invariant on interfaceName... this is mostly b/c the PUT method doesn't require any of these inputs. We use NetworkInterfaceUpdate to drive initialValues which has name typed as string | null | undefined. The modal is invoked like below, notice that initialValues is potentially an empty object.
<EditNetworkInterfaceSideModalForm
isOpen={!!editing}
initialValues={editing || {}}
onDismiss={() => setEditing(null)}
/>
|
Edit: this is unrelated to this PR, made it its own issue #993 These lines in the mock NIC create endpoint (which were already there) mean that any NIC create without a VPC name and a subnet name will 404. console/libs/api-mocks/msw/handlers.ts Lines 491 to 499 in 1f746d5 Turns out these are not actually optional on the POST, which is a relief because I did not understand how that would work. So those fields should be marked required in the create form. |
|
I'm going to merge to move forward, but again I have #991 for updates. If anything else comes up in post review, let me know and I'll add it to that issue as a follow up. |
4 participants
Fixes #933
Changes
It's notable that
Primaryis disabled if the NIC you're editing is already primary. @benjaminleonard this could probably use a better design. Should I put a field title over it?TODO